-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make pwn template always set context.binary #2279
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This might help simple usecases of just invoking pwn template
.
This feels like we need to improve the documentation around how to use the template generator tool. When using the plain invocation instead of providing the target binary during pwn template ./challenge
invocation, the path has to be edited into the generated script manually, which seems redundant and like a misuse of the template generator :(
This also shifts the requirement to change the generated script right away to remote-only exploit templates generated using pwn template --host abc.com --port 1234
since it'd try to access the ./path/to/binary
dummy file now, so you'd have to comment out that line and remember to add the context.update(arch='XXX')
back manually.
Maybe the best compromise would be to keep the current output of not using context.binary
if there is no binary given and host
is set.
Thanks for the feedback! What's the preferred development approach here? Should changes from the review stay as a separate commit or be squashed into the first one? |
Separate commits are fine for such small changes like this. We squash the whole PR branch before merging. |
Is #2309 an acceptable solution to this problem too? |
@peace-maker Yes, that patch sounds good. However, this still leaves he old behavior when the challenge files could not be determined automatically. But this is probably good enough |
We can still add the change to include |
@peace-maker I just added a commit that implements the fallback you suggested for remote-only challenges. Sorry that it took me this long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I like it!
pwn
template only setcontext.binary
when an argument was specified and only defined a placeholder path instead. It would be more reasonable to setcontext.binary
even if no path was specified as pwntools can automatically determine the correct settings for the binary when you fill in the placeholder path.Old template without specifying target binary as argument:
New template without specifying target binary as argument:
Partially resolves #2276